Skip to content

vscode: migrate to more type-safe assert impl #3372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Feb 28, 2020
Merged

vscode: migrate to more type-safe assert impl #3372

merged 6 commits into from
Feb 28, 2020

Conversation

@Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Feb 28, 2020 •
edited
Loading

@Veetaha
Copy link
Contributor Author

Veetaha commented Feb 28, 2020

@lnicola, @matklad , why can't I add reviewers anymore???
image

@lnicola lnicola requested review from lnicola and matklad and removed request for lnicola February 28, 2020 21:58
lnicola
lnicola reviewed Feb 28, 2020
View reviewed changes
Copy link
Member

@lnicola lnicola left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. I've spotted three uses of assert:

  • one checks that the Content-Length header from GitHub is a number
    • but GitHub could very well use Transfer-Encoding: chunked without any Content-Length header, so this should not be an assertion; we could show an indeterminate progress bar if that happens
  • one checks that the binary runs
    • but we could have uploaded a wrong binary to GitHub, or the extension might have downloaded the wrong one (ahem Linux/ARM)
  • one checks that we didn't get an error when creating the destination directory
    • but this might as well happen in day-to-day usage (e.g. when running out of disk space)

I'm used to assertions meaning "this couldn't have happened unless the program state is inconsistent", instead of "this bad thing outside of our control happened and we wish it hadn't".

So the last two of those cases would probably be served better by a notification, and the first is pretty harmless but should probably be handled for robustness.

matklad
matklad reviewed Feb 28, 2020
View reviewed changes
editors/code/src/util.ts Outdated
import * as vscode from "vscode";
import { strict as nativeAssert } from "assert";

export function assert(condition: unknown, explanation: string): asserts condition {
Copy link
Contributor

@matklad matklad Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a single assert(condition: boolean, explanation: string) should be enough.

Copy link
Contributor Author

@Veetaha Veetaha Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Copy link
Contributor Author

@Veetaha Veetaha Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, do you mean boolean? If yes, we would need to explicitly use null comparisongs assert(obj == null) vs assert(obj), or is this desired?

Copy link
Contributor

@matklad matklad Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think explicit boolean is good

Copy link
Contributor Author

@Veetaha Veetaha Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@Veetaha
Copy link
Contributor Author

Veetaha commented Feb 28, 2020 •
edited
Loading

@lnicola
Though I am always for robustness so that we write the code that is statically strongly-typed and all-errors-cases-proven, for our VSCode this might not be the case. Protocol errors, sanity type checks (1, 2), naming invariants violation, hardware exhaustion (though I do have out-of hard-drive memory errors with the number of terabytes Rust debug builds occupy on my 68GB-space laptop), etc... handling all of these gracefully would bloat the codebase which we aspire to keep as small and concise as we can.

Even that assert(!Number.isNan(contentLength)) already seems excess as to me...

Anyway, we should be able to catch assertion errors and display them to the user. In fact assert in TypeScript doesn't crash the process, but instead throws AssertionError, just like we would

throw new Error("Sanity check was not successful");

@Veetaha Veetaha requested a review from matklad February 28, 2020 22:46
@Veetaha Veetaha force-pushed the fix/inlays-change-revert-no-updated branch from 27e561d to 6dc598f Compare February 28, 2020 22:59
@matklad
Copy link
Contributor

matklad commented Feb 28, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 28, 2020

Build succeeded

@bors bors bot merged commit 7cf710c into rust-lang:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lnicola lnicola lnicola left review comments

@matklad matklad Awaiting requested review from matklad

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Veetaha @matklad @lnicola